Skip to content

tapd: add itest for grouped asset channel funding #987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

guggero
Copy link
Member

@guggero guggero commented Feb 26, 2025

@guggero
Copy link
Member Author

guggero commented Apr 15, 2025

Updated go.mod to point to the now merged lightninglabs/taproot-assets#1413.
Ready for review.

@guggero guggero requested review from ffranr and removed request for gijswijs April 15, 2025 17:36
@lightninglabs-deploy
Copy link

@GeorgeTsagk: review reminder
@ffranr: review reminder

@guggero
Copy link
Member Author

guggero commented Apr 23, 2025

Ready for review, @ffranr and @GeorgeTsagk.

guggero added 2 commits April 24, 2025 11:55
We remove the Zane node from all tests where its only role was being the
universe server. Starting a node takes multiple seconds, so if we can
spin up fewer nodes, we can save some time.
And functionality wise Charlie can easily be the universe, we just need
to define its port upfront so we can configure it to be its own proof
courier.
@levmi levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Apr 24, 2025
@guggero guggero force-pushed the group-key-support branch 3 times, most recently from 2ce063a to 23f300d Compare April 25, 2025 08:17
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Non-blocking: Since the group keys on taprpc are already merged, we could enhance testCustomChannelsLiquidityEdgeCasesCore with an extra option similar to groupMode, which would control whether we open single asset ID or group funded channels. This way we could incorporate low-hanging coverage for this PR. Not sure what the diff for that would be, could def be a follow-up PR.

guggero added 6 commits April 29, 2025 15:02
This commit prepares some of the helper functions to be able to handle
channels that have multiple asset pieces in them.
We'll be adding a new HTLC force close test where we'll be able to
re-use this functionality.
@guggero guggero force-pushed the group-key-support branch from 23f300d to 21e38f3 Compare April 29, 2025 13:02
@guggero
Copy link
Member Author

guggero commented Apr 29, 2025

Addressed comments.

Comment on lines +113 to +115
// We use Charlie as the proof courier. But in order for Charlie to also
// use itself, we need to define its port upfront.
charliePort := port.NextAvailablePort()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here and in the git commit message (or next to the topology map) we can mention that charlie is effectively a remote universe server for Erin/Fabia. And maybe the same for the other tests.

Comment on lines +345 to +372
// Then we burn everything but a single asset piece.
assetID1 := mintedAssets[0].AssetGenesis.AssetId
assetID2 := mintedAssets[1].AssetGenesis.AssetId
burnAmount1 := mintedAssets[0].Amount - charlieFundingAmount/2 -
erinFundingAmount/2 - 1
_, err := charlieTap.BurnAsset(ctx, &taprpc.BurnAssetRequest{
Asset: &taprpc.BurnAssetRequest_AssetId{
AssetId: assetID1,
},
AmountToBurn: burnAmount1,
ConfirmationText: taprootassets.AssetBurnConfirmationText,
})
require.NoError(t.t, err)

mineBlocks(t, net, 1, 1)

burnAmount2 := mintedAssets[1].Amount - charlieFundingAmount/2 -
erinFundingAmount/2 - 1
_, err = charlieTap.BurnAsset(ctx, &taprpc.BurnAssetRequest{
Asset: &taprpc.BurnAssetRequest_AssetId{
AssetId: assetID2,
},
AmountToBurn: burnAmount2,
ConfirmationText: taprootassets.AssetBurnConfirmationText,
})
require.NoError(t.t, err)

mineBlocks(t, net, 1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing this. Please consider adding a bit more to the comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we burn all but a small piece as we don't support fully burning an asset in a single transaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

5 participants